PPSC-1037: Add auth login/logout/whoami with OAuth2 device flow#257
Open
dmitrysmirnov-armis wants to merge 5 commits into
Open
Conversation
|
| Severity | Count |
|---|---|
| 🟠 HIGH | 1 |
| 🟡 MEDIUM | 1 |
Total: 2
View all 2 findings
🟠 HIGH (1)
CWE-327 - Cryptography Failures (CWE-327
Location: internal/auth/device.go:258
Use of Broken or Risky Cryptographic Algorithm): The program talks to the authentication server and receives a JWT access token. It then calls parseAccessTokenClaims to read the token’s contents, but it never checks the token’s digital signature. Because the signature isn’t verified, anyone who can supply a forged token could make the client believe the token contains whatever user, role, or tenant information they want. The client would then use those false claims for things like deciding which tenant it belongs to or what permissions it has. Since the token comes from a network request, an attacker who can influence that response could exploit this weakness. Adding proper signature verification (or using a library that does it automatically) would prevent the client from trusting tampered tokens.
Code snippet is redacted as it contains secrets.
CWEs: CWE-327: Use of Broken or Risky Cryptographic Algorithm
🟡 MEDIUM (1)
CWE-253 - In the `runAuthLogout` function the code calls `store.Load(env)` and assigns the result to `existing`, but it completely ignores the second return value (the error)
Location: internal/cmd/auth_logout.go:53
In the runAuthLogout function the code calls store.Load(env) and assigns the result to existing, but it completely ignores the second return value (the error). If store.Load fails—for example because the token file is missing, corrupted, or the program lacks permission—the error is discarded and the program proceeds as if nothing was stored. This means a problem with loading the credentials can go unnoticed, and the user will see a misleading message that no credentials were removed. Because this command is part of a command‑line tool that any user can run, the mistake can be triggered simply by invoking the logout command. There is no additional check or fallback to handle a load failure, so the vulnerable code may hide real errors and give a false sense of security. To fix the issue, the code should examine the error returned by store.Load and handle it appropriately (e.g., report the failure to the user or abort the operation). This ensures that problems with reading stored credentials are not silently ignored.
return nil
}
fmt.Fprintf(os.Stderr, "%s Signed out of %d environment(s).\n", output.IconSuccess, len(envs))
return nil
}
env := getAPIBaseURL()
// Report whether anything was actually stored, so logout is informative
// rather than silently succeeding when not logged in.
existing, _ := store.Load(env)
if err := store.Clear(env); err != nil {
return fmt.Errorf("failed to remove stored credentials: %w", err)
}
if existing == nil {
fmt.Fprintf(os.Stderr, "No stored credentials to remove for %s.\n", env)
return nil
}
fmt.Fprintf(os.Stderr, "%s Signed out of %s.\n", output.IconSuccess, env)
return nil
CWEs: CWE-253: Incorrect Check of Function Return Value
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Test Coverage Reporttotal: (statements) 73.8% Coverage by function |
There was a problem hiding this comment.
Pull request overview
Adds interactive SSO authentication to armis-cli using the OAuth2 Device Authorization Grant, with a shared on-disk session store for reuse across CLI runs and MCP plugins, plus supporting commands to manage and inspect the session.
Changes:
- Introduces
armis-cli auth login/logout/whoamiand keeps the previous raw-token behavior as hiddenauth token. - Implements OAuth2 device-flow client, persisted token store (
~/.armis/.sessions), and SSO token refresh + provider wiring. - Updates auth resolution to prefer stored SSO sessions and optionally auto-trigger device login via
ARMIS_DEFAULT_AUTH_METHOD=SSO.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents recommended auth methods (CI client-credentials vs interactive SSO) and session storage details. |
| internal/cmd/scan_repo.go | Passes command context into auth provider resolution to support interactive login lifetime. |
| internal/cmd/scan_image.go | Passes command context into auth provider resolution to support interactive login lifetime. |
| internal/cmd/root.go | Adds stored-token precedence, SSO auto-login gating, and improved no-credentials messaging. |
| internal/cmd/root_test.go | Updates auth-provider tests for new context.Context signature. |
| internal/cmd/auth.go | Makes auth a visible command group and moves legacy token-print behavior to hidden auth token. |
| internal/cmd/auth_whoami.go | Adds auth whoami for inspecting current auth method/identity/tenant/expiry. |
| internal/cmd/auth_test.go | Adjusts auth command tests for new error messages and SSO token-store isolation. |
| internal/cmd/auth_logout.go | Adds auth logout with environment scoping and --all. |
| internal/cmd/auth_login.go | Adds auth login command and shared device-flow login helper. |
| internal/cmd/auth_flow_test.go | Adds end-to-end tests for login/whoami/logout and SSO auto-login gating. |
| internal/cmd/agent_detection_collect.go | Updates auth provider call to accept a context. |
| internal/auth/tokenstore.go | Implements cross-process token persistence in ~/.armis/.sessions. |
| internal/auth/tokenstore_test.go | Adds coverage for token store behavior, permissions, env scoping, and corruption handling. |
| internal/auth/oauth_provider_test.go | Adds tests for SSO provider refresh behavior and persisted rotation. |
| internal/auth/device.go | Implements OAuth2 device-flow + refresh-token client with proxy/HTTPS/redirect hardening. |
| internal/auth/device_test.go | Adds unit tests for device-flow request/poll/refresh and claim parsing. |
| internal/auth/client.go | Extracts shared URL scheme constants for HTTPS enforcement logic. |
| internal/auth/browser.go | Adds safe browser opener with scheme validation and test override hook. |
| internal/auth/browser_test.go | Adds tests for browser URL validation and opener invocation. |
| internal/auth/auth.go | Extends AuthProvider to support SSO mode, identity/expiry reporting, and refresh-token rotation persistence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+77
to
+79
| browseURL := da.VerificationURIComplete | ||
| opened := auth.OpenBrowser(browseURL) == nil | ||
| printVerificationInstructions(da, browseURL, opened) |
Comment on lines
+226
to
+243
| // write persists the entries to the 0600 file, creating ~/.armis (0700) if needed. | ||
| func (s *TokenStore) write(entries []tokenEntry) error { | ||
| path, err := s.filePath() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| data, err := json.MarshalIndent(entries, "", " ") //nolint:gosec // G117: persisting the token blob to its file IS the purpose of this store | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal tokens: %w", err) | ||
| } | ||
| if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { | ||
| return fmt.Errorf("failed to create token directory: %w", err) | ||
| } | ||
| if err := os.WriteFile(path, data, 0o600); err != nil { //nolint:gosec // path derived from os.UserHomeDir + hardcoded segments | ||
| return fmt.Errorf("failed to write token file: %w", err) | ||
| } | ||
| return nil | ||
| } |
Comment on lines
+511
to
+518
| func augmentNoCredentialsError(err error) error { | ||
| if err == nil || !strings.Contains(err.Error(), "authentication required") { | ||
| return err | ||
| } | ||
| return fmt.Errorf("not authenticated — use one of the following options:\n" + | ||
| " - run 'armis-cli auth login' to sign in with your company IdP\n" + | ||
| " - or set ARMIS_CLIENT_ID / ARMIS_CLIENT_SECRET (or --client-id / --client-secret) for JWT auth\n" + | ||
| " - or set ARMIS_API_TOKEN (or --token) for legacy auth") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
Type of Change
Problem
The CLI only supported static credentials (client-credentials JWT and a legacy API token). Developers had no way to authenticate via browser-based SSO against their corporate IdP, and there was no shared session that the CLI and the Armis MCP plugins could both reuse from a single sign-in.
Solution
Implements the
authcommand group built on the OAuth2 Device Authorization Grant (RFC 8628):auth login— requests a device code (POST /oauth2/device,client_iddefaults toarmis-cli), auto-opens the browser atverification_uri_complete(falls back to printing the URL +user_codeover SSH/headless), then pollsPOST /oauth2/tokenhonoringauthorization_pending/slow_down/expired_token/access_denied. On success it stores the token and prints the identity and tenant.auth logout— clears the stored token for the current environment;--allclears every environment.auth whoami— shows environment, auth method, identity, tenant, region, and token expiry.ARMIS_CLIENT_ID/ARMIS_CLIENT_SECRET→ legacyARMIS_API_TOKEN→ an error pointing atauth login. Explicit--client-id/--client-secret/--tokenoverride the stored token (escape hatch). CI/CD env-var auth is unchanged.Token storage is a
0600JSON file at~/.armis/.sessions(a JSON array of{env, token}keyed by the resolved API base URL), giving a single source of truth that the Python MCP plugins can share —go-keyringdoes not interoperate with Python'skeyring, and the backend's refresh-token reuse-detection makes a divergent second store dangerous.Changes layered on top of the original ticket scope:
auth login/logout/whoami). The originalauth(raw JWT from client credentials) is preserved as a hiddenauth tokensubcommand.ARMIS_DEFAULT_AUTH_METHODenv var — set toSSOto auto-trigger the device-flow login on the first command that needs credentials when none are configured (requiresARMIS_TENANT_ID/--tenant-id). Gated so it never shadows a working CI/service-account setup.ARMIS_DEFAULT_AUTH_METHOD.Testing
Automated Tests
New/updated coverage: device-flow client (
device_test.go), token store (tokenstore_test.go), OAuth provider refresh (oauth_provider_test.go), browser opener (browser_test.go), and the end-to-end command flow including the SSO auto-login gating and trigger (auth_flow_test.go).Manual Testing
go build ./...,go test ./internal/cmd/ ./internal/auth/, andgolangci-lint runall pass locally. End-to-end login/whoami/logout exercised against a mock OAuth2 device-flow server inauth_flow_test.go.Reviewer Notes
getAuthProvidernow takes acontext.Contextso the interactive auto-login can run for the command's full lifetime rather than a short request timeout.Checklist